Leaf Proof Count 7#106
Conversation
n13
left a comment
There was a problem hiding this comment.
Please address the bugbot issue - can just rename the binaries or version them
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2525b29. Configure here.
|
I've reviewed PR #106 thoroughly — fetched the branch locally, read the diffs, the hand-written code, the build/runtime logic, the dependency changes, and the bugbot/reviewer threads. Here's my assessment. OverallSolid, well-reasoned PR. It does two largely independent things: (1) retargets ZK circuit generation from 16 → 7 leaf proofs per batch for mobile memory, and (2) regenerates Status of prior review items
Ok(config) => config.num_leaf_proofs == env_num_leaf_proofs(),This is the right fix and correctly forces regeneration for installed users who already have 16-leaf bins.
Issues worth addressing1. Operational (highest risk): the leaf count must match the deployed runtime. Going 16 → 7 changes the aggregation circuit and therefore its verifier key. If the on-chain aggregation verifier was not deployed with 2. // No `cargo:rerun-if-changed` directives: Cargo uses default fingerprinting,
// re-running this script when any source file in the package changes.
...
println!("cargo:rerun-if-env-changed=QP_NUM_LEAF_PROOFS");Once you emit any 3. Crate version not bumped ( 4. Batch-size source of truth is inconsistent (DRY / correctness). let batch_size = agg_config.num_leaf_proofs;but .chunks(DEFAULT_NUM_LEAF_PROOFS)If 5. Stale dissolve UX/docs. So 6. Breaking library/SDK surface (note for changelog). Removed public items: 7. Dependency cleanup is partial. Dropping the direct Nice work
RecommendationApprove-with-changes. Before merge I'd want: confirmation that the target chain runtime uses Want me to push fixes for the quick ones (#2 comment, #4 batch-size unification, #3 version bump, #5 dissolve removal) on the PR branch? I won't commit or comment on GitHub without your go-ahead. |
|
4, 5, and 7 should be fixed IMO |

also updated metadata to latest for chain
this needs to update to latest version of qp-zk-circuits
Note
High Risk
Large generated metadata and circuit-parameter changes can break installed binaries until bins regenerate and must match the deployed runtime; multisig/high-security API removals are breaking for older integrations.
Overview
This PR retargets wormhole/ZK circuit generation for 7 leaf proofs per batch (down from 16), aimed at lower peak memory on mobile, and bumps the CLI to
qp-poseidon-core2.0.2 while dropping the separateqp-poseidondependency.Circuit binaries:
DEFAULT_NUM_LEAF_PROOFSis now 7 and is public for reward aggregation. Runtimeis_ready()also treats cachedgenerated-binsas stale whenconfig.json’snum_leaf_proofsor the CLI version marker no longer match;build.rsonly watchesQP_NUM_LEAF_PROOFSfor rebuilds. Aclippy.shscript mirrors CI’s locked clippy invocation.Chain metadata:
quantus_subxt.rsis regenerated for the latest runtime—ZkTreepallet/API,zk_tree_rooton block headers, wormhole events carryingleaf_index(Merkle proofs via RPC), removal of wormholeTransferProofstorage, reversible-transfersguardiannaming/storage (GuardianIndex,next_transaction_id), and multisig pallet changes (e.g. noapprove_dissolve, bounded propose calls,SignerApprovedevent,MaxInnerCallWeight). The high-security CLI followsguardian_index; the multisig example printsproposal_nonceinstead of active proposal count.Note: Example text may still mention dissolve APIs that no longer exist on-chain until follow-up doc fixes.
Reviewed by Cursor Bugbot for commit a6d2324. Bugbot is set up for automated code reviews on this repo. Configure here.